fix: add onPermissionRequest handler to sessions across various docum…#892
fix: add onPermissionRequest handler to sessions across various docum…#892horihiro wants to merge 3 commits intogithub:mainfrom
Conversation
…entation and examples
There was a problem hiding this comment.
Pull request overview
This PR updates the Node.js/TypeScript documentation and examples to consistently show handling of session permission prompts by adding onPermissionRequest (often via the approveAll helper) when creating Copilot sessions.
Changes:
- Add
onPermissionRequestto session creation examples across docs and the Node example. - Update imports in TypeScript snippets to include
approveAllwhere referenced. - Modernize/standardize session config snippets to reflect the required permission-handling pattern.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| nodejs/examples/basic-example.ts | Adds approveAll and passes it via onPermissionRequest when creating a session. |
| nodejs/README.md | Updates multiple README snippets to include onPermissionRequest and introduces approveAll usage. |
| docs/troubleshooting/debugging.md | Adds onPermissionRequest to a debugging snippet for session creation. |
| docs/troubleshooting/compatibility.md | Adds onPermissionRequest to an infinite-sessions configuration example. |
| docs/setup/scaling.md | Adds onPermissionRequest to scaling-oriented session creation examples. |
| docs/setup/local-cli.md | Updates local CLI setup snippet to import/use approveAll and adds onPermissionRequest to another example. |
| docs/setup/github-oauth.md | Updates OAuth setup snippet to import/use approveAll and pass onPermissionRequest. |
| docs/setup/bundled-cli.md | Updates bundled CLI setup snippet to import/use approveAll and adds onPermissionRequest to more configs. |
| docs/setup/backend-services.md | Updates backend-service examples to import/use approveAll and pass onPermissionRequest. |
| docs/setup/azure-managed-identity.md | Updates managed identity example to import/use approveAll and pass onPermissionRequest. |
| docs/hooks/user-prompt-submitted.md | Adds onPermissionRequest to multiple hook examples. |
| docs/hooks/session-lifecycle.md | Adds onPermissionRequest to session lifecycle hook examples (incl. one using approveAll). |
| docs/hooks/pre-tool-use.md | Adds onPermissionRequest to pre-tool-use hook examples. |
| docs/hooks/post-tool-use.md | Adds onPermissionRequest to post-tool-use hook examples. |
| docs/hooks/index.md | Updates hooks overview snippet to import/use approveAll and adds onPermissionRequest elsewhere. |
| docs/hooks/error-handling.md | Adds onPermissionRequest to error-handling hook examples. |
| docs/getting-started.md | Updates getting-started snippets to import/use approveAll and add onPermissionRequest. |
| docs/features/steering-and-queueing.md | Updates import to include approveAll for permission-handling consistency. |
| docs/features/skills.md | Adds onPermissionRequest to a skills configuration example. |
| docs/features/session-persistence.md | Updates import to include approveAll and adds onPermissionRequest to a persistence example. |
| docs/features/mcp.md | Updates MCP examples to import/use approveAll and pass onPermissionRequest. |
| docs/features/custom-agents.md | Adds onPermissionRequest to custom agent session creation examples. |
| docs/auth/byok.md | Updates BYOK examples to import/use approveAll and pass onPermissionRequest in multiple configs. |
| return null; | ||
| }, | ||
| }, | ||
| onPermissionRequest: approveAll |
There was a problem hiding this comment.
This snippet uses onPermissionRequest: approveAll, but approveAll isn’t defined or imported within the code block. Since docs code blocks are validated as standalone TypeScript files, this will fail compilation. Either add the appropriate import (approveAll from @github/copilot-sdk) to the block, or switch this line to an inline handler like async () => ({ kind: "approved" }) to keep the snippet self-contained.
| onPermissionRequest: approveAll | |
| onPermissionRequest: async () => ({ kind: "approved" }) |
nodejs/README.md
Outdated
|
|
||
| ```typescript | ||
| import type { PermissionRequest, PermissionRequestResult } from "@github/copilot-sdk"; | ||
| import type { PermissionRequest, PermissionRequestResult, approveAll } from "@github/copilot-sdk"; |
There was a problem hiding this comment.
In this snippet, approveAll is a runtime value export, but it’s being imported via import type { ... }, which will fail TypeScript compilation. Import approveAll with a normal import { approveAll } ... (and keep PermissionRequest/PermissionRequestResult as type-only if desired).
| import type { PermissionRequest, PermissionRequestResult, approveAll } from "@github/copilot-sdk"; | |
| import { approveAll } from "@github/copilot-sdk"; | |
| import type { PermissionRequest, PermissionRequestResult } from "@github/copilot-sdk"; |
| @@ -712,6 +722,7 @@ const session = await client.createSession({ | |||
|
|
|||
| return { kind: "approved" }; | |||
| }, | |||
| onPermissionRequest: approveAll | |||
| }); | |||
There was a problem hiding this comment.
This createSession object literal defines onPermissionRequest twice (custom handler at the top, then onPermissionRequest: approveAll later). Duplicate keys are a TypeScript error and it’s unclear which behavior the example intends. Remove one of the two, or split into two separate examples (custom handler vs approveAll).
docs/getting-started.md
Outdated
| model: "gpt-4.1", | ||
| streaming: true, | ||
| }); | ||
| }, onPermissionRequest: approveAll); |
There was a problem hiding this comment.
This createSession call is syntactically invalid: onPermissionRequest is being passed outside the config object (}, onPermissionRequest: approveAll);). Move onPermissionRequest: approveAll inside the object literal, and ensure approveAll is imported in this snippet (the import currently only brings in CopilotClient).
|
|
||
| ```typescript | ||
| import { CopilotClient } from "@github/copilot-sdk"; | ||
| import { CopilotClient, approveAll } from "@github/copilot-sdk"; |
There was a problem hiding this comment.
I notice there's a mix of approveAll and inline handlers (async () => ({ kind: "approved" })) across many of these docs. For this one in particular it seems strange to import approveAll and then not use it.
Would you be willing to check across all these docs and standardize on one approach? Ideally let's use the approveAll import for all of them.
|
@horihiro Looks like there are a few syntactical errors across these changes (see the comments). Would you be open to updating this to address them? Thanks! (Note: I clicked the close button by mistake, so have also reopened this PR!) |
|
@SteveSandersonMS Unified all |
This pull request updates documentation examples across multiple files to consistently demonstrate how to handle permission requests when creating Copilot sessions. The primary change is the addition of the
onPermissionRequestparameter—often using the newapproveAllhelper—to all relevant TypeScript/Node.js sample code. This ensures users see the recommended way to handle permission prompts in their own implementations.The most important changes include:
Documentation consistency and clarity:
onPermissionRequestparameter to allCopilotClient.createSessionexamples in the documentation, demonstrating both inline async handlers and the newapproveAllhelper for automatic approval. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22] [23] [24] [25]Import updates for new helper:
approveAllfrom@github/copilot-sdkalongsideCopilotClient(anddefineToolwhere used), ensuring the helper is available in code samples. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]Sample code modernization:
These changes improve the clarity and correctness of the documentation, ensuring users follow best practices for permission handling in Copilot SDK integrations.…entation and examples